Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Compatibility] Added COMMAND GETKEYS and GETKEYSANDFLAGS command #888

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Vijay-Nirmal
Copy link
Contributor

@Vijay-Nirmal Vijay-Nirmal commented Dec 18, 2024

Adding the COMMAND GETKEYS and GETKEYSANDFLAGS command to garnet

  • Add COMMAND GETKEYS and GETKEYSANDFLAGS commands
  • Add Integration Test cases and ACL Test
  • Add documentation

Copy link
Contributor

@TalZaccai TalZaccai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing this! Added a few comments.

libs/server/Resp/BasicCommands.cs Outdated Show resolved Hide resolved
libs/server/Resp/BasicCommands.cs Outdated Show resolved Hide resolved
libs/server/Resp/BasicCommands.cs Outdated Show resolved Hide resolved
libs/server/Resp/BasicCommands.cs Outdated Show resolved Hide resolved
return AbortWithErrorMessage(CmdStrings.RESP_COMMAND_HAS_NO_KEY_ARGS);
}

var keyList = new List<byte[]>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have to allocate a byte[] for each key? The keys already exist in the parse state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically you don't even need to allocate the list, the keys are always consecutive in the parse state, right? If so, you only need the first and last index of keys...

libs/server/Resp/BasicCommands.cs Outdated Show resolved Hide resolved
libs/server/Resp/BasicCommands.cs Outdated Show resolved Hide resolved
libs/server/Resp/BasicCommands.cs Outdated Show resolved Hide resolved
libs/server/Resp/BasicCommands.cs Outdated Show resolved Hide resolved
playground/CommandInfoUpdater/CommandDocsUpdater.cs Outdated Show resolved Hide resolved
@Vijay-Nirmal
Copy link
Contributor Author

Fixed all review commands

Copy link
Contributor

@TalZaccai TalZaccai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the recent fixes! Added some extra comments.

@@ -1485,6 +1485,50 @@
"ArgumentFlags": "Optional, Multiple"
}
]
},
{
"Command": "COMMAND_GETKEYS",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here about running CommentInfoUpdater - #864 (comment)

/// <summary>
/// Extension methods for the SessionParseState struct.
/// </summary>
internal static class SessionParseStateExtension
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods can be added to the existing Garnet.server/SessionParseStateExtensions.cs

return true;
}

internal static bool TryExtractKeyandFlagsFromSpecs(this ref SessionParseState state, RespCommandKeySpecification[] keySpecs, out List<ArgSlice> keys, out List<string[]> flags)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming: TryExtractKeysAndFlagsFromSpecs

if (startIndex < 0 || startIndex >= state.Count)
return false;

if (spec.FindKeys is FindKeysRange range)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this type matching you can have ExtractKeys defined as an abstract method in FindKeysKeySpecMethodBase

{
int startIndex = 0;

if (spec.BeginSearch is BeginSearchIndex bsIndex)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too, you can have an abstract method called GetStartIndex (or something similar) in BeginSearchKeySpecMethodBase that you can implement for each of the different types.

@@ -263,6 +279,32 @@ public BeginSearchKeyword(string keyword, int startFrom) : this()
this.Keyword = keyword;
this.StartFrom = startFrom;
}

public bool TryGetStartFrom(ref SessionParseState parseState, out int index)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing XML comment


// Determine the search direction
int increment = StartFrom < 0 ? -1 : 1;
int start = StartFrom < 0 ? searchStartIndex : searchStartIndex;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mistake?

@@ -339,6 +381,35 @@ public FindKeysRange(int lastKey, int keyStep, int limit) : this()
this.KeyStep = keyStep;
this.Limit = limit;
}

public void ExtractKeys(ref SessionParseState state, int startIndex, List<ArgSlice> keys)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing XML comment

@@ -385,6 +456,45 @@ public FindKeysKeyNum(int keyNumIdx, int firstKey, int keyStep) : this()
this.FirstKey = firstKey;
this.KeyStep = keyStep;
}

public void ExtractKeys(ref SessionParseState state, int startIndex, List<ArgSlice> keys)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing XML comment

// Extract keys based on numKeys, firstKey, and keyStep
if (numKeys > 0 && firstKey >= 0)
{
for (int i = 0; i < numKeys && firstKey + i * KeyStep < state.Count; i++)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Add parentheses around i * KeyStep for clarity (in the next line as well)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants